Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend documentation on server-side apply #14300

Closed
wants to merge 3 commits into from

Conversation

kwiesmueller
Copy link
Member

This PR adds more documentation for server-side apply and serves as the placeholder for potential other additions that are targeted for 1.15.

Current additions:

  • Documentation on how to clear the managedFields object manually.

/milestone 1.15
/sig api-machinery
/wg apply
/cc @apelisse @jennybuckley @simplytunde

@k8s-ci-robot
Copy link
Contributor

@kwiesmueller: You must be a member of the kubernetes/website-milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Website milestone maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

This PR adds more documentation for server-side apply and serves as the placeholder for potential other additions that are targeted for 1.15.

Current additions:

  • Documentation on how to clear the managedFields object manually.

/milestone 1.15
/sig api-machinery
/wg apply
/cc @apelisse @jennybuckley @simplytunde

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 13, 2019
@k8s-ci-robot k8s-ci-robot requested a review from apelisse May 13, 2019 16:44
@k8s-ci-robot k8s-ci-robot requested a review from jennybuckley May 13, 2019 16:44
@k8s-ci-robot
Copy link
Contributor

@kwiesmueller: GitHub didn't allow me to request PR reviews from the following users: simplytunde.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This PR adds more documentation for server-side apply and serves as the placeholder for potential other additions that are targeted for 1.15.

Current additions:

  • Documentation on how to clear the managedFields object manually.

/milestone 1.15
/sig api-machinery
/wg apply
/cc @apelisse @jennybuckley @simplytunde

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2019
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented May 13, 2019

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit f6601f0

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5cffef7713e94d00071bb2c3

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels May 13, 2019
@kwiesmueller kwiesmueller changed the title extended documentation on server-side apply extend documentation on server-side apply May 13, 2019
@tengqm
Copy link
Contributor

tengqm commented May 14, 2019

Please indicate if this is a WIP.

@kwiesmueller kwiesmueller changed the title extend documentation on server-side apply [WIP] extend documentation on server-side apply May 14, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2019
@kwiesmueller
Copy link
Member Author

sorry @tengqm
yes it's wip for now.


### Clearing ManagedFields

It is possible to strip all managedFields from an object by overwriting them using `MergePatch`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it only work with mergepatch?

@makoscafee
Copy link
Contributor

/milestone 1.15

@k8s-ci-robot k8s-ci-robot added this to the 1.15 milestone May 29, 2019
@apelisse
Copy link
Member

I think this is still relevant for 1.15. @kwiesmueller would you mind following-up on that?

@kwiesmueller
Copy link
Member Author

Sure, I'll take care of it.

@kwiesmueller kwiesmueller changed the title [WIP] extend documentation on server-side apply extend documentation on server-side apply Jun 4, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2019
@kwiesmueller
Copy link
Member Author

@apelisse I added another example for using JSONPatch. The PR should be ready for review by EOD today to still make it for 1.15. Can you think of any further things we should add here and get ready by this evening?


### Clearing ManagedFields

It is possible to strip all managedFields from an object by overwriting them using `MergePatch` or `JSONPatch`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about smp, what about updates?

@makoscafee
Copy link
Contributor

makoscafee commented Jun 11, 2019

@kwiesmueller Perfect, its just that this KEP is targeted for 1.16 I think kubernetes/enhancements#555 (comment). Correct me if I am wrong. It did not make it to 1.15 release cut.
cc/ @apelisse for more information.

@makoscafee
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2019
@kwiesmueller
Copy link
Member Author

@makoscafee the feature we talk about is already in alpha since 1.14. This PR got added to address a question as part of our umbrella issue kubernetes/kubernetes#73723 to show people how to reset a field that got introduced in 1.14 (alpha).

So adding this is only related to the feature inside the KEP but directly targets the current release (1.15) on purpose.
The test proving the documentation added here works as intended got added here: kubernetes/kubernetes#77821

@makoscafee
Copy link
Contributor

Thanks for clarification @kwiesmueller. Do you think will it be possible for it to be merged by today? we are behind schedule on getting all PR for this release merged.

Let me know if there is anything needed.

/hold clear

@kwiesmueller
Copy link
Member Author

@apelisse would it be okay for you to merge this?
I think all other options (except apply) should work but couldn't test them yet. If we wan't to document all the options, I'd prefer having tests for them as well and then add them to the docs which won't happen in this release due to code freeze (i think).

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jimangel
You can assign the PR to them by writing /assign @jimangel in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kwiesmueller
Copy link
Member Author

@apelisse I updated the PR and added this one for kubernetes/kubernetes#78912 verifying the docs work in all cases.

@apelisse
Copy link
Member

Thanks a lot Kevin, looking at it now (I ended-up in various meeting this morning so I couldn't exactly do it when I said I would)

@apelisse
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2019
@apelisse
Copy link
Member

@jimangel Did you mean /approve? :-)

@apelisse
Copy link
Member

One thing that is not super clear is ... what does it mean to strip all the managed fields? what about the operation that you're running as you clear?

Like.. what if I do:

PATCH /api/v1/namespaces/default/configmaps/example-cm
Content-Type: application/merge-patch+json
Accept: application/json
Data: {"metadata":{"managedFields": [{}]}, "other": "changes"}

What is managedField going to look like after this patch? Will it be empty or will it be me owning other?

@kwiesmueller
Copy link
Member Author

I just tested this, seems like when you overwrite the managedFields in a request, this is the final operation. So in your example, the managedFields are empty.
We really got to document this, so I'll add it. But maybe we also should discuss if this is how we want it to behave?

@apelisse
Copy link
Member

I agree that seems like a bug to me.

@makoscafee
Copy link
Contributor

Hi, @kwiesmueller and @apelisse so since this PR has some work to be done. and the deadline to have all PR passed since June 10th.I am going to strip it from 1.15 milestone and we can get it in as a fast follow -- retarget for master after the release, since we are publishing continuously from master.

@makoscafee
Copy link
Contributor

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the 1.15 milestone Jun 12, 2019
@zacharysarah zacharysarah force-pushed the dev-1.15 branch 3 times, most recently from 9031cd5 to 4a57e78 Compare June 17, 2019 21:24
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@makoscafee
Copy link
Contributor

Closing this PR as it is referencing dev-1.15and the cycle has ended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants